fix: reach did not terminate on nightly, and depth alone was never going to bound it - #30
Merged
Merged
Conversation
…ing to bound it My own regression, and the way it reached main is worth writing down. #29 removed a `MethodError` that `reach` threw on `sum(f(x) for x in xs)`. That throw was also, accidentally, a terminator. Without it, on 1.14.0-DEV: reach(f, Tuple{Vector{Float64}}) # [f(x) for x in xs] — did not return reach(g, Tuple{Vector{Float64}}) # sum(map(f, xs)) — did not return Both answer in milliseconds on 1.12.2. The nightly leg is `continue-on-error`, so it could not block the merge; I then cancelled the run that was sitting in `runtest` and a background job merged #29 the moment `gh pr checks` reported nothing pending. The hang was on main for about forty minutes. `maxdepth` bounds how FAR the walk goes, not how much of it there is. Thirty-two levels branching by sixteen candidates is not a finite amount of work in any useful sense, and `visited` only prunes signatures that repeat — a higher-order call generates new ones. So the walk now also carries a `maxwork` budget, shared with every subwalk, and spends `:unknown` with `why = :budget` when it runs out. That is what `:unknown` is for; the alternative was a call that never comes back. Shared, not per-branch, and the distinction is load-bearing: `visited` is deliberately reset in `_subwalk` so a candidate reached under another branch is still walked here, which means `visited` cannot also be the thing that bounds the total. `maxwork` is a keyword on `reach`, `reach(::Module)` and `reach_script`, with the measurement in the docstring — a caller whose entry point comes back `:unknown` with a `:budget` is in a different situation from one that is genuinely dynamic, and only they can decide to pay for more. The spec now pins the property that does not move between versions, because the verdict does: `[unstable(x) for x in xs]` is `:depends` on 1.12.2 and `:unknown` on 1.14.0-DEV. What must hold everywhere is that a caller which can reach a mark is never reported `:clean` — with a control that the same shapes with nothing marked behind them still are, so the assertion is not satisfied by an analysis that never says `:clean` at all. Measured on both: 1237 assertions on 1.12.2, 1236 on 1.14.0-DEV, 190 behaviours, green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
📚 Docs preview: https://codes.sota-shimozono.com/ExperimentalAPI.jl/previews/PR30/ (updates on each push to this PR) |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…ys so Measured while answering "what happens with several marks behind one loop": budgets between "too small to reach any" and the default report `:depends` with one, two, … of twelve found and the rest never walked to. The verdict is right either way; the LIST is not complete, and `truncated = true` is the only thing that says so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
My own regression, and how it reached main is part of the report.
#29 removed a
MethodErrorthatreachthrew onsum(f(x) for x in xs). That throw was also,accidentally, a terminator. Without it, on 1.14.0-DEV:
Both answer in milliseconds on 1.12.2.
The nightly leg is
continue-on-error, so it could not block the merge. I then cancelled the runthat had been sitting in
runtestfor 40 minutes, and a background job merged #29 the momentgh pr checksreported nothing pending. The hang was on main for about forty minutes.The fix, and why the existing bound was not one
maxdepthbounds how far the walk goes, not how much of it there is. Thirty-two levels branchingby sixteen candidates is not a finite amount of work in any useful sense, and
visitedonly prunessignatures that repeat — a higher-order call generates new ones.
The walk now carries a
maxworkbudget, shared with every subwalk, and spends:unknownwithwhy = :budgetwhen it runs out.Shared rather than per-branch, and the distinction is load-bearing:
visitedis deliberately resetin
_subwalkso a candidate reached under another branch is still walked here — which meansvisitedcannot also be the thing that bounds the total.maxworkis a keyword onreach,reach(::Module)andreach_script, with the measurement inthe docstring. An entry point that comes back
:unknownwith a:budgetinunresolvedis adifferent situation from one that is genuinely dynamic, and only the caller can decide to pay more.
The spec now pins what does not move
The verdict does move:
[unstable(x) for x in xs]is:dependson 1.12.2 and:unknownon1.14.0-DEV, because the budget runs out there first.
:unknownis weaker;:cleanwould be false.So the assertions are:
:clean:clean:cleanpassesmaxwork = 1gives:unknownwithwhy === :budgetMeasured on both: 1237 assertions on 1.12.2, 1236 on 1.14.0-DEV, 190 behaviours, green.
🤖 Generated with Claude Code